Conversation
sid-rl
left a comment
There was a problem hiding this comment.
passing in DevboxView instead of id into the Devbox constructor (and having from_id make an API call) is a significant change and a departure from how the rest of the OO SDK does things. we should discuss design and make sure we think through our placement of api calls/how we cache properties before we make such a change. blocking for now
|
If tunnel (and potentially other properties) need to be on the object at creation time, the data has to come from somewhere. Passing the full The alternative, where having a tunnel property that's sometimes None and requires a separate Also, there's 0 users of the object oriented SDK, so now's the time to make this change |
| :return: Wrapper bound to the requested devbox | ||
| :rtype: AsyncDevbox | ||
| """ | ||
| return AsyncDevbox(self._client, devbox_id) |
There was a problem hiding this comment.
Also see pydoc above referencing intentionally being a quick return with option for user to await retrieve
|
At the moment the objects are a reference to the remote object, in this case for speed we could cache the tunnel and then have a getter for it which if it's not cached would go collect it (in a fromId situation). Performing this change in this way introduces caching issues, when things change and it changes the meaning of fromId. IMO lets keep all of that the same and we can cache on positive for the hostname and collect if it's required and we don't have it. Making the critical path fast without having to deal with caching issues or making fromId unnecessarily make calls. |
|
I feel like lazy loading will lead to inconsistent object states. For example, a devbox from create() has tunnel immediately, one from if we add a Also, it seems pretty rare that people just have an ID. Usually it comes from a a previous API call and you're probably calling another API anyways |
|
My $0.02: we so far have a couple of implementation detail suggestions (return filled out objects vs wrappers that represent server-side state) and some hints at design philosophies (reduce remote calls, avoid complexity from caching). The SDK calls right now are following Alex's original design, which has its merits but which was never spelled out or vetted in a design review. This current PR would represent a partial change to that design in one part of the SDK, which would be ill-advised and create confusion. We might benefit from changing the design of the OO SDKs (and @Albert - you are right that now is the time to do so if we want to), but we should go into that with eyes wide open rather than ad-hoc. So.... my recommendation is that we should pause this part of this PR, have a proper design discussion, and then move forward with implementing things once we have an agreed-upon direction. I asked Sid to put together a straw-man proposal for the SDKs caches-vs-always-query logic earlier this evening. We can use that as a starting point for this design conversation. |
This PR does the following:
AsyncDevbox/Devboxconstructors now takeDevboxViewwith tunnel propertyAsyncScenarioRun.devboxproperty → asyncget_devbox()methodview_tunnel()method added to network interface